bindspace→mailbox_soa migration: W0 dependency map + W1 small-column migration (additive, parity-tested)#517
Conversation
|
Warning Review limit reached
More reviews will be available in 52 minutes and 44 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds three new D-MBX-A2 per-row columns ( ChangesMailboxSoA D-MBX-A2 columns and migration preflight
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/cognitive-shader-driver/src/mailbox_soa.rs (1)
826-899: ⚡ Quick winAdd a focused reset test for the new A2 columns.
The parity test is solid, but it does not verify
reset_row()clearstemporal/expert/sigma. A small targeted test will protect migration invariants against regressions.Suggested test addition
+ #[test] + fn test_mailbox_soa_reset_row_clears_a2_columns() { + let mut mb: MailboxSoA<4> = MailboxSoA::new(1, 0, 1.0); + mb.set_temporal(2, 123); + mb.set_expert(2, 77); + mb.set_sigma(2, 9); + + mb.reset_row(2); + + assert_eq!(mb.temporal_at(2), 0, "temporal[2] must reset to 0"); + assert_eq!(mb.expert_at(2), 0, "expert[2] must reset to 0"); + assert_eq!(mb.sigma_at(2), 0, "sigma[2] must reset to 0"); + }As per coding guidelines, "
crates/**/*.rs: Add Rust unit tests alongside implementations via#[cfg(test)]modules; prefer focused scenarios over broad integration tests."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/cognitive-shader-driver/src/mailbox_soa.rs` around lines 826 - 899, The parity test test_mailbox_soa_column_parity_with_bindspace() verifies that temporal, expert, and sigma values are correctly set and retrieved, but it does not validate that the reset_row() method clears these new A2 columns. Add a new focused unit test with the #[test] attribute in the same module that specifically tests the reset_row() behavior for the A2 columns by setting values for temporal, expert, and sigma in a MailboxSoA instance, calling reset_row() on specific rows, and then asserting that these fields return to their default/zero state to protect against migration invariants regressions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/plans/bindspace-mailbox-soa-dependency-map-v1.md:
- Around line 27-32: The document contains contradictory status markers for
three columns: fingerprints.sigma, temporal, and expert are marked as **GAP** in
the A2-gap mapping table (around lines 27-32, 43-44, and 143-148), but the W1
"DONE" section indicates these columns are already shipped. Locate the W1 "DONE"
section to verify the actual shipping status of these three columns, then update
all occurrences of their status markers in the earlier A2-gap sections from
**GAP** to **SHIPPED** (or whatever status marker is used in the W1 section) to
resolve the contradiction and keep migration tracking consistent throughout the
document.
---
Nitpick comments:
In `@crates/cognitive-shader-driver/src/mailbox_soa.rs`:
- Around line 826-899: The parity test
test_mailbox_soa_column_parity_with_bindspace() verifies that temporal, expert,
and sigma values are correctly set and retrieved, but it does not validate that
the reset_row() method clears these new A2 columns. Add a new focused unit test
with the #[test] attribute in the same module that specifically tests the
reset_row() behavior for the A2 columns by setting values for temporal, expert,
and sigma in a MailboxSoA instance, calling reset_row() on specific rows, and
then asserting that these fields return to their default/zero state to protect
against migration invariants regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 077474ba-206e-42d5-844b-3d4b29fe79f6
📒 Files selected for processing (2)
.claude/plans/bindspace-mailbox-soa-dependency-map-v1.mdcrates/cognitive-shader-driver/src/mailbox_soa.rs
…eflight
Read-based map (full reads of every critical-path consumer + two Opus inventory
agents for the rest) produced BEFORE any wiring, per operator directive ("map
the many dependencies before"; "two paths step by step, never delete the old
before testing the new"; "CausalEdge64 is duplicated — no handwaving").
Contents:
- §0 the two stores side by side; the D-MBX-A2 gap (content/topic/angle dense
hot per OQ-1-resolved, sigma, temporal, expert); cycle (Vsa16kF32) = DROP.
- §1 the CausalEdge64 duplication mapped exactly: causal_edge::edge (SPO baton,
used everywhere) vs ndarray::hpc::causal_diff (weight-diff codec, NEVER
imported into lance-graph). Both repr(transparent)/u64 → silent-corruption
hazard at the u64 level. The contract's raw-u64 edges_raw() is the firewall;
typed reattach only at MailboxSoA owner + p64-bridge. v2 layout drops temporal.
- §2 consumer map: column hot-spots (engine_bridge + driver), the 2 bin
construction sites, the 4 Arc::get_mut single-owner hazards, the singleton-
bound tests, and the ALREADY-built cold side (surreal_container
SurrealMailboxView impls MailboxSoaView; scheduler generic over it; the
driver already holds both bindspace + mailboxes).
- §3 W0–W7 delete-last wiring sequence (parity test W1, differential test W2,
feature-gated engine_bridge/dispatch swap W3/W4, bins W5, tombstone W6,
delete BindSpace LAST W7).
- §4 dedup hazards checklist.
No source wired. Branch off main (post-#516), independent of the open #515.
Co-Authored-By: Claude <noreply@anthropic.com>
…ve, parity-tested) First wiring step of the bindspace→mailbox_soa arc. ADDITIVE only — the singleton BindSpace is untouched; nothing deleted (operator: "two paths step by step, never delete the old before testing the new"). - MailboxSoA<N> gains temporal: [u64;N], expert: [u16;N], sigma: [u8;N] (the D-MBX-A2 small columns) + accessors (temporal_at/set_temporal/…) + reset_row. temporal is a standalone column NOT folded into the edge: the v2 CausalEdge64 layout dropped temporal bits (I-LEGACY-API-FEATURE-GATED). sigma is a Σ-codebook index (reference, not content; I-VSA-IDENTITIES). - test_mailbox_soa_column_parity_with_bindspace: writes matched per-row values to a BindSpace window and a MailboxSoA, asserts edges/qualia/meta/entity_type + temporal/expert/sigma read back identically. The "test the new" proof. - 13 mailbox_soa tests green, clippy clean. W1b (dense content/topic/angle hot planes — the driver's resonance read) is the next step; the cycle (Vsa16kF32) plane is never migrated (OQ-1/§2.7). Dependency map + W0–W7 sequence: bindspace-mailbox-soa-dependency-map-v1.md. Co-Authored-By: Claude <noreply@anthropic.com>
CodeRabbit review on #517, both valid: - doc: sigma/temporal/expert were marked **GAP** in the §0 mapping table while the W1 DONE section said shipped — contradiction. Updated the three rows to **SHIPPED (W1)** and the "remaining D-MBX-A2 gap" line to content/topic/angle only (the W1b heap planes). - test: added test_mailbox_soa_reset_row_clears_a2_columns — reset_row() must clear the new temporal/expert/sigma columns (migration-invariant regression guard). 14 mailbox_soa tests green. Also rebased onto post-#515 main (e063416) so W1b will wire against the merged driver (which now carries the MaterializeProvenance provenance wire). Co-Authored-By: Claude <noreply@anthropic.com>
AdaWorldAPI
force-pushed
the
claude/bindspace-mailbox-soa-migration
branch
from
9ddca83 to
fc7849c
Compare
2 participants
What
Opens the bindspace → mailbox_soa migration arc. This PR is W0 (dependency map) + W1 (first additive column migration) — strictly additive, parity-tested, nothing deleted. The singleton
BindSpaceis fully intact; both paths run.Operator constraints honored: the replacement is a given; two parallel paths, step by step — the old is never deleted before the new is tested; map the dependencies before wiring;
CausalEdge64duplication handled precisely, no handwaving.W0 — dependency map / wiring preflight (
.claude/plans/bindspace-mailbox-soa-dependency-map-v1.md)Read-based (full reads of every critical-path consumer + two Opus inventory agents for the rest), produced before any wiring:
causal_edge::edge::CausalEdge64(the SPO/thinking baton — the EdgeColumn, used everywhere) vsndarray::hpc::causal_diff::CausalEdge64(a weight-diff codec — never imported into lance-graph, 0 sites). Both#[repr(transparent)]overu64with public.0and overlapping method names of incompatible semantics ⇒ the hazard is purelyu64-level. The contract's raw-u64MailboxSoaView::edges_raw()is the firewall (keeps the contract zero-dep); typed reattach lives at exactly two trusted boundaries (MailboxSoAowner via the const-assertedrepr(transparent)cast +p64-bridge). Rule locked: the ndarray twin is barred from the baton path. v2 layout dropstemporal⇒ it stays a standalone column.surreal_container::SurrealMailboxViewalready implementsMailboxSoaView(read-only, rawu64);LanceVersionScheduleris generic over it; the driver already holds bothbindspaceand amailboxesmap (the two-path cradle).Arc::get_mut(&mut bindspace)single-owner sites;attention_mask*is a separateAttentionMaskSoA(must not conflate);cycle/Vsa16kF32never migrates;content/topic/anglestay hot (OQ-1 resolved §2.7). The 2BindSpace::zeros(4096)bin construction sites.BindSpaceLAST (W7).W1 — migrate the small columns (
temporal/expert/sigma)MailboxSoA<N>gainstemporal: [u64;N],expert: [u16;N],sigma: [u8;N]+ accessors +reset_rowcoverage.temporalis a standalone column, not folded into the edge (the v2CausalEdge64reclaimed those bits —I-LEGACY-API-FEATURE-GATED).sigmais a Σ-codebook index (a reference, not content —I-VSA-IDENTITIES; the codebook stays shared/cold).test_mailbox_soa_column_parity_with_bindspace: writes matched per-row values to aBindSpacewindow and aMailboxSoA, then assertsedges/qualia/meta/entity_type(D-MBX-A1) +temporal/expert/sigma(W1) read back identically. This is the "test the new before deleting the old" proof.Tests
13
mailbox_soatests green (parity test included);clippy -p cognitive-shader-driver --libclean. No dispatch path touched;BindSpaceuntouched.Next (not in this PR)
W1b — the dense
content/topic/anglehot identity planes (the driver's resonance read; heapBox<[u64]>,cycleexcluded). Then W2–W7 per the map. Each step stays two-path and delete-last.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
New Features
Tests